deps: introduce embedder version number for V8 #9754

targos · 2016-11-23T09:54:13Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

V8

Description of change

Refs: #9730

targos · 2016-11-23T09:56:39Z

/cc @nodejs/v8

targos · 2016-11-23T09:58:54Z

deps/v8/test/cctest/test-version.cc

-  CheckVersion(2, 5, 10, 7, false, "2.5.10.7 SIMULATOR", "libv8-2.5.10.7.so");
-  CheckVersion(2, 5, 10, 7, true,
-               "2.5.10.7 (candidate) SIMULATOR", "libv8-2.5.10.7-candidate.so");
+  CheckVersion(1, 0, 0, 1, 0, false, "1.0.0.1.0 SIMULATOR", "libv8-1.0.0.1.0.so");


I know there are some long lines. Will fix them if we decide to go further

bnoordhuis · 2016-11-23T10:15:13Z

LGTM modulo style issues. Can you add a line or two to the commit log explaining why this change is introduced?

versionCheck() in lib/internal/v8_prof_polyfill.js needs an update after this, you could add that as a separate commit.

It should handle both 6 and 7 tuple records because of ./configure --without-bundled-v8.

MylesBorins · 2016-11-23T10:16:18Z

is this something we should backport? should we be using it for all patches
moving forward?

On Wed, Nov 23, 2016, 6:15 PM Ben Noordhuis notifications@github.com
wrote:

LGTM modulo style issues. Can you add a line or two to the commit log
explaining why this change is introduced?

versionCheck() in lib/internal/v8_prof_polyfill.js needs an update after
this, you could add that as a separate commit.

It should handle both 6 and 7 tuple records because of ./configure
--without-bundled-v8.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#9754 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV8DVuHHqPsufNOWa8Ow8yiZUJ6g0ks5rBBI1gaJpZM4K6Y1m
.

rvagg · 2016-11-23T10:20:06Z

How about we just draw a line here and say from this point onward we're going to be bumping this number whenever we float stuff but leave previous branches as they are and patch how we have been. I can't think of why people would be parsing the full version number of V8 but you could argue it's a breaking change to extend it.

targos · 2016-11-23T10:27:03Z

Yeah I agree this is a major change

rvagg · 2016-11-23T10:48:22Z

Major change but we'll need it in v7 for #9730 won't we or are we going to get by with a clash with that one?

Fishrock123 · 2016-11-23T15:51:46Z

Could this be upstreamed to V8?

ofrobots · 2016-11-23T15:53:45Z

Yep, I would like to see this upstreamed too, once we are happy on our side /cc @natorion.

addaleax · 2016-11-23T15:55:10Z

In that case, should the suffix maybe be something embedder-specified? Like, x.y.z.w.node1 for our versions?

targos · 2016-11-27T10:33:16Z

Fixed the style issues and added a comment to explain the change.

versionCheck() in lib/internal/v8_prof_polyfill.js needs an update after this, you could add that as a separate commit.

I don't think it needs to be changed. The function only checks the first 3 numbers of the version string.

In that case, should the suffix maybe be something embedder-specified? Like, x.y.z.w.node1 for our versions?

I implemented it as a separate commit (so it's easy to revert). I had to remove embedder_ from base::hash_combine(major_, minor_, build_, patch_)); because there is no hash function for the char* type.
The version number looks like this now:

./node -p 'process.versions.v8'
5.4.500.43.node.0

bnoordhuis · 2016-11-28T07:38:25Z

The function only checks the first 3 numbers of the version string.

Yes, but it also checks that line.split(',').length === 6.

jasnell · 2016-12-05T23:05:04Z

Needs to be rebased and updated now!

targos · 2016-12-06T08:41:25Z

Updated, but the conflict confirmed something I feared. If we add this line in v8-version.h, we can no longer directly apply a patch from upstream.
So I put it below V8_IS_CANDIDATE_VERSION instead.

bnoordhuis · 2016-12-06T10:39:09Z

deps/v8/include/v8-version.h

@@ -17,4 +17,7 @@
 // (Boolean macro values are not supported by all preprocessors.)
 #define V8_IS_CANDIDATE_VERSION 0

+// Modify this when changes are made by the embedder.
+#define V8_EMBEDDER_VERSION "node.0"


I think it's nicer to write this like this:

#ifndef V8_EMBEDDER_STRING #define V8_EMBEDDER_STRING "" #endif // V8_EMBEDDER_STRING

And then override it at build time. That would also make it more likely for this patch to get accepted upstream.

override it at build time

You mean in node.gyp ?

I like the idea but I'm not sure how to update the other files. Should I add conditions everywhere to check if the embedder string is empty ?

I'm not quite sure what you mean. You'd check that GetEmbedder() and V8_EMBEDDER_STRING are equal.

Just make it the responsibility of the embedder to include the dot: ['defines': 'V8_EMBEDDER_STRING=".node.0"'].

bnoordhuis · 2016-12-06T10:43:09Z

lib/internal/v8_prof_polyfill.js

  // whereas process.versions.v8 is either "$major.$minor.$build" or
  // "$major.$minor.$build.$patch".
  var firstLine = readline();
  line = firstLine + '\n' + line;
  firstLine = firstLine.split(',');
  const curVer = process.versions.v8.split('.');
-  if (firstLine.length !== 6 && firstLine[0] !== 'v8-version') {
+  if (![6, 7].includes(firstLine.length) || firstLine[0] !== 'v8-version') {


This is IMO less legible than (firstLine.length !== 6 && firstLine.length !== 7).

targos · 2016-12-06T17:10:16Z

node.gyp

@@ -236,6 +236,7 @@
        'NODE_WANT_INTERNALS=1',
        # Warn when using deprecated V8 APIs.
        'V8_DEPRECATION_WARNINGS=1',
+        'V8_EMBEDDER_STRING=".node.0"'


This doesn't seem to work. Should it be somewhere else ?

common.gypi or config.gypi? With common.gypi it's kind of gnarly that it ends up being defined everywhere, including add-ons.

targos · 2016-12-06T17:12:17Z

deps/v8/src/log-utils.cc

@@ -56,9 +56,9 @@ void Log::Initialize(const char* log_file_name) {

    if (output_handle_ != nullptr) {
      Log::MessageBuilder msg(this);
-      msg.Append("v8-version,%d,%d,%d,%d,%d", Version::GetMajor(),
+      msg.Append("v8-version,%d,%d,%d,%d,%s,%d", Version::GetMajor(),


What should I do here ?

You mean print unconditionally or only when it's a non-empty string? The former is easier but really, it's up to you.

Sometimes upstream V8 may not want to merge a fix to their stable branches, but we might. This adds a new version string that the embedder can set independently of the official V8 version to avoid running into conflicts. Refs: nodejs#9730

Update the processor to accept a V8 with or without an embedder version string.

targos · 2016-12-07T17:56:08Z

I found a way to define it in common.gypi without polluting everything else.
I know I have to make a similar addition to BUILD.gn or something but I'd like some feedback on the approach, especially from @ofrobots because I'm not convinced it is the best way to go.
In the future, we hope to be able to maintain our copies of V8 in the nodejs/v8 repo. Aren't we going to directly add our version string there? In that case editing v8-version.h seems like the simplest action.

targos · 2016-12-12T13:54:40Z

Ping ^

bnoordhuis

LGTM. Completely untested but I expect the GN equivalent looks like this:

diff --git a/BUILD.gn b/BUILD.gn
index 71ebc5c..013e96f 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -26,6 +26,8 @@ declare_args() {
   # Enable compiler warnings when using V8_DEPRECATED apis.
   v8_deprecation_warnings = false
 
+  v8_embedder_string = ""
+
   # Enable compiler warnings when using V8_DEPRECATE_SOON apis.
   v8_imminent_deprecation_warnings = ""
 
@@ -171,6 +173,9 @@ config("features") {
 
   defines = []
 
+  if (v8_embedder_string != "") {
+    defines += [ "V8_EMBEDDER_STRING=\"$v8_embedder_string\"" ]
+  }
   if (v8_enable_disassembler) {
     defines += [ "ENABLE_DISASSEMBLER" ]
   }

ofrobots · 2016-12-12T18:10:10Z

@natorion: any thoughts on the above change that adds embedder version to the V8 version string? This will solve one of the problems we have discussed in the past with Node.js needing to float a patch to V8 stable/beta that V8 is unwilling to merge upstream. It would be to good to get this change upstream.

It would be curious to see if the ecosystem is tightly dependent on the V8 version string format. Here's a CITGM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/474/

natorion · 2016-12-12T19:24:19Z

I will run this idea by our infra folks internally. Out of my head: This will break a few services in V8's infra, which we should prepare before this is upstreamed.

ofrobots · 2016-12-12T21:23:57Z

@natorion I don't believe the default version format changes upstream. Embedders are able to add an embedder string to the version if they want to.

natorion · 2016-12-14T11:13:16Z

I create an upstream tracking bug: https://bugs.chromium.org/p/v8/issues/detail?id=5740.

Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set on compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "5.5.372.37.custom.1", the embedder must set V8_EMBEDDER_STRING to ".custom.1". Related Node.js issue: nodejs/node#9754 BUG=v8:5740 R=machenbach@chromium.org,hablich@chromium.com,ofrobots@google.com Review-Url: https://codereview.chromium.org/2619213002 Cr-Commit-Position: refs/heads/master@{#42175}

…id:20001 of https://codereview.chromium.org/2619213002/ ) Reason for revert: Seems to break the Chromium build: https://codereview.chromium.org/2619193005/ Message: [1832/9671] CXX obj/v8/v8_base/version.o FAILED: obj/v8/v8_base/version.o /b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/v8/v8_base/version.o.d -DV8_DEPRECATION_WARNINGS -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DUSE_PROPRIETARY_CODECS -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DV8_I18N_SUPPORT -DENABLE_HANDLE_ZAPPING -DV8_USE_EXTERNAL_STARTUP_DATA -DV8_TARGET_ARCH_X64 -DDEBUG -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -I../.. -Igen -I../../v8 -I../../v8/include -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -fcolor-diagnostics -fdebug-prefix-map=/b/c/b/linux/src=. -m64 -march=x86-64 -pthread -g1 --sysroot=../../build/linux/debian_wheezy_amd64-sysroot -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Werror -Wall -Wno-unused-variable -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Wsign-compare -Winconsistent-missing-override -Wshorten-64-to-32 -O3 -fno-ident -fdata-sections -ffunction-sections -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -fno-rtti -fno-exceptions -Wno-deprecated -c ../../v8/src/version.cc -o obj/v8/v8_base/version.o ../../v8/src/version.cc:42:34: error: use of undeclared identifier 'V8_EMBEDDER_STRING' const char* Version::embedder_ = V8_EMBEDDER_STRING; ^ 1 error generated. Original issue's description: > [build] Introduce an embedder version string > > Sometimes, the embedder might want to merge a fix to an abandoned branch > or to a supported branch but the fix is not relevant to Chromium. > This adds a new version string that the embedder can set on compile time > and that will be appended to the official V8 version. > The separator must be provided in the string. For instance, to have a > full version string like "5.5.372.37.custom.1", the embedder must set > V8_EMBEDDER_STRING to ".custom.1". > > Related Node.js issue: nodejs/node#9754 > > BUG=v8:5740 > R=machenbach@chromium.org,hablich@chromium.com,ofrobots@google.com > > Review-Url: https://codereview.chromium.org/2619213002 > Cr-Commit-Position: refs/heads/master@{#42175} > Committed: https://chromium.googlesource.com/v8/v8/+/fc86d4329b253bf21c1dd85469f1ef4b6e5ba01a TBR=hablich@chromium.com,machenbach@chromium.org,ofrobots@google.com,mic.besace@gmail.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=v8:5740 Review-Url: https://codereview.chromium.org/2621033002 Cr-Commit-Position: refs/heads/master@{#42182}

targos · 2017-01-21T17:26:41Z

This moved to a V8 CL here: https://codereview.chromium.org/2619213002/
Closing.

Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set on compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "5.5.372.37.custom.1", the embedder must set V8_EMBEDDER_STRING to ".custom.1". Related Node.js issue: nodejs/node#9754 BUG=v8:5740 R=machenbach@chromium.org,hablich@chromium.com,ofrobots@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng Review-Url: https://codereview.chromium.org/2619213002 Cr-Original-Commit-Position: refs/heads/master@{#42175} Committed: https://chromium.googlesource.com/v8/v8/+/fc86d4329b253bf21c1dd85469f1ef4b6e5ba01a Review-Url: https://codereview.chromium.org/2619213002 Cr-Commit-Position: refs/heads/master@{#42582}

…id:40001 of https://codereview.chromium.org/2619213002/ ) Reason for revert: Blocks roll https://codereview.chromium.org/2647183002/ Original issue's description: > [build] Introduce an embedder version string > > Sometimes, the embedder might want to merge a fix to an abandoned branch > or to a supported branch but the fix is not relevant to Chromium. > This adds a new version string that the embedder can set on compile time > and that will be appended to the official V8 version. > The separator must be provided in the string. For instance, to have a > full version string like "5.5.372.37.custom.1", the embedder must set > V8_EMBEDDER_STRING to ".custom.1". > > Related Node.js issue: nodejs/node#9754 > > BUG=v8:5740 > R=machenbach@chromium.org,hablich@chromium.com,ofrobots@google.com > > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng > > Review-Url: https://codereview.chromium.org/2619213002 > Cr-Original-Commit-Position: refs/heads/master@{#42175} > Committed: https://chromium.googlesource.com/v8/v8/+/fc86d4329b253bf21c1dd85469f1ef4b6e5ba01a > Review-Url: https://codereview.chromium.org/2619213002 > Cr-Commit-Position: refs/heads/master@{#42582} > Committed: https://chromium.googlesource.com/v8/v8/+/2c1d1e60883882011ed50310a9b09e95dc61234a TBR=hablich@chromium.com,machenbach@chromium.org,ofrobots@google.com,mic.besace@gmail.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=v8:5740 Review-Url: https://codereview.chromium.org/2643393004 Cr-Commit-Position: refs/heads/master@{#42583}

Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set at compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "6.0.287.53-emb.1", the embedder must set V8_EMBEDDER_STRING to "-emb.1". Related Node.js issue: nodejs/node#9754 BUG=v8:5740 R=machenbach@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ifa2d9bd213795e6d54886436f8c3787ac6162823 Reviewed-on: https://chromium-review.googlesource.com/690475 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Michaël Zasso <mic.besace@gmail.com> Cr-Commit-Position: refs/heads/master@{#48301}

Original commit message: [build] Introduce an embedder version string Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set at compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "6.0.287.53-emb.1", the embedder must set V8_EMBEDDER_STRING to "-emb.1". Related Node.js issue: nodejs#9754 BUG=v8:5740 R=machenbach@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ifa2d9bd213795e6d54886436f8c3787ac6162823 Reviewed-on: https://chromium-review.googlesource.com/690475 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Michaël Zasso <mic.besace@gmail.com> Cr-Commit-Position: refs/heads/master@{nodejs#48301}

Original commit message: [build] Introduce an embedder version string Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set at compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "6.0.287.53-emb.1", the embedder must set V8_EMBEDDER_STRING to "-emb.1". Related Node.js issue: #9754 BUG=v8:5740 R=machenbach@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ifa2d9bd213795e6d54886436f8c3787ac6162823 Reviewed-on: https://chromium-review.googlesource.com/690475 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Michaël Zasso <mic.besace@gmail.com> Cr-Commit-Position: refs/heads/master@{#48301} Refs: v8/v8@b096c44 PR-URL: #15785 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Original commit message: [build] Introduce an embedder version string Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set at compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "6.0.287.53-emb.1", the embedder must set V8_EMBEDDER_STRING to "-emb.1". Related Node.js issue: nodejs#9754 BUG=v8:5740 R=machenbach@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ifa2d9bd213795e6d54886436f8c3787ac6162823 Reviewed-on: https://chromium-review.googlesource.com/690475 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Michaël Zasso <mic.besace@gmail.com> Cr-Commit-Position: refs/heads/master@{nodejs#48301} Refs: v8/v8@b096c44 PR-URL: nodejs#15785 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Original commit message: [build] Introduce an embedder version string Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set at compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "6.0.287.53-emb.1", the embedder must set V8_EMBEDDER_STRING to "-emb.1". Related Node.js issue: #9754 BUG=v8:5740 R=machenbach@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ifa2d9bd213795e6d54886436f8c3787ac6162823 Reviewed-on: https://chromium-review.googlesource.com/690475 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Michaël Zasso <mic.besace@gmail.com> Cr-Commit-Position: refs/heads/master@{#48301} Refs: v8/v8@b096c44 PR-URL: #15785 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Original commit message: [build] Introduce an embedder version string Sometimes, the embedder might want to merge a fix to an abandoned branch or to a supported branch but the fix is not relevant to Chromium. This adds a new version string that the embedder can set at compile time and that will be appended to the official V8 version. The separator must be provided in the string. For instance, to have a full version string like "6.0.287.53-emb.1", the embedder must set V8_EMBEDDER_STRING to "-emb.1". Related Node.js issue: nodejs/node#9754 BUG=v8:5740 R=machenbach@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ifa2d9bd213795e6d54886436f8c3787ac6162823 Reviewed-on: https://chromium-review.googlesource.com/690475 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Michaël Zasso <mic.besace@gmail.com> Cr-Commit-Position: refs/heads/master@{#48301} Refs: v8/v8@b096c44 PR-URL: nodejs/node#15785 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

targos added the v8 engine Issues and PRs related to the V8 dependency. label Nov 23, 2016

targos mentioned this pull request Nov 23, 2016

deps: cherry-pick 08377af from v8 upstream #9730

Closed

3 tasks

targos commented Nov 23, 2016

View reviewed changes

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 23, 2016

targos force-pushed the v8-version-embedder branch from 6887875 to 88120f1 Compare November 27, 2016 10:27

targos force-pushed the v8-version-embedder branch from 88120f1 to 8d41a22 Compare November 29, 2016 19:51

targos force-pushed the v8-version-embedder branch 2 times, most recently from 02626e3 to 98c6c02 Compare December 6, 2016 08:40

bnoordhuis reviewed Dec 6, 2016

View reviewed changes

targos force-pushed the v8-version-embedder branch 2 times, most recently from 4192a60 to f21cdd1 Compare December 6, 2016 17:06

targos commented Dec 6, 2016

View reviewed changes

targos added 3 commits December 7, 2016 18:45

deps: introduce embedder version string for V8

6a7aa65

Sometimes upstream V8 may not want to merge a fix to their stable branches, but we might. This adds a new version string that the embedder can set independently of the official V8 version to avoid running into conflicts. Refs: nodejs#9730

build: add V8 embedder version string

48265fe

tools: update tick processor for V8 embedder version

6240a57

Update the processor to accept a V8 with or without an embedder version string.

targos force-pushed the v8-version-embedder branch from f21cdd1 to 6240a57 Compare December 7, 2016 17:48

bnoordhuis approved these changes Dec 12, 2016

View reviewed changes

targos closed this Jan 21, 2017

targos deleted the v8-version-embedder branch January 21, 2017 17:26

targos mentioned this pull request Sep 30, 2017

add a node suffix to V8's version number #15698

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: introduce embedder version number for V8 #9754

deps: introduce embedder version number for V8 #9754

targos commented Nov 23, 2016

targos commented Nov 23, 2016

targos Nov 23, 2016

bnoordhuis commented Nov 23, 2016

MylesBorins commented Nov 23, 2016

rvagg commented Nov 23, 2016

targos commented Nov 23, 2016

rvagg commented Nov 23, 2016

Fishrock123 commented Nov 23, 2016

ofrobots commented Nov 23, 2016

addaleax commented Nov 23, 2016

targos commented Nov 27, 2016

bnoordhuis commented Nov 28, 2016

jasnell commented Dec 5, 2016

targos commented Dec 6, 2016 •

edited

Loading

bnoordhuis Dec 6, 2016

targos Dec 6, 2016

bnoordhuis Dec 6, 2016

targos Dec 6, 2016

bnoordhuis Dec 6, 2016

bnoordhuis Dec 6, 2016

targos Dec 6, 2016

bnoordhuis Dec 6, 2016

targos Dec 6, 2016

bnoordhuis Dec 6, 2016

targos commented Dec 7, 2016

targos commented Dec 12, 2016

bnoordhuis left a comment

ofrobots commented Dec 12, 2016

natorion commented Dec 12, 2016

ofrobots commented Dec 12, 2016

natorion commented Dec 14, 2016

targos commented Jan 21, 2017

deps: introduce embedder version number for V8 #9754

deps: introduce embedder version number for V8 #9754

Conversation

targos commented Nov 23, 2016

Checklist

Affected core subsystem(s)

Description of change

targos commented Nov 23, 2016

Choose a reason for hiding this comment

bnoordhuis commented Nov 23, 2016

MylesBorins commented Nov 23, 2016

rvagg commented Nov 23, 2016

targos commented Nov 23, 2016

rvagg commented Nov 23, 2016

Fishrock123 commented Nov 23, 2016

ofrobots commented Nov 23, 2016

addaleax commented Nov 23, 2016

targos commented Nov 27, 2016

bnoordhuis commented Nov 28, 2016

jasnell commented Dec 5, 2016

targos commented Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Dec 7, 2016

targos commented Dec 12, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

ofrobots commented Dec 12, 2016

natorion commented Dec 12, 2016

ofrobots commented Dec 12, 2016

natorion commented Dec 14, 2016

targos commented Jan 21, 2017

targos commented Dec 6, 2016 •

edited

Loading